Fix NPE with ApiKeyPair during listApis call (from cmk)#13149
Fix NPE with ApiKeyPair during listApis call (from cmk)#13149sureshanaparti wants to merge 3 commits into
Conversation
|
@KlausDornsbach can you review this. |
|
@blueorangutan package |
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #13149 +/- ##
=============================================
- Coverage 18.08% 3.46% -14.63%
=============================================
Files 6037 479 -5558
Lines 542611 41162 -501449
Branches 66433 7793 -58640
=============================================
- Hits 98136 1426 -96710
+ Misses 433448 39543 -393905
+ Partials 11027 193 -10834
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17820 |
@sureshanaparti, @KlausDornsbach is not active in the community anymore. I am asking @bernardodemarco to review this, as he was responsible for maintaning the keypairs PR. |
|
@sureshanaparti, thanks for the PR! I'll try to review and test it during this week. |
|
@blueorangutan test |
|
@blueorangutan test |
|
@sureshanaparti a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16105)
|
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
kiranchavala
left a comment
kiranchavala
left a comment
There was a problem hiding this comment.
Getting the following error on ol8
(localcloud) 🐱 > sync
panic: interface conversion: interface {} is nil, not []interface {}
goroutine 1 [running]:
github.com/apache/cloudstack-cloudmonkey/config.(*Config).UpdateCache(0xc0000e0120?, 0xc0013b0390)
/home/shwstppr/lab/shapeblue/cloudstack-cloudmonkey/config/cache.go:115 +0x1338
github.com/apache/cloudstack-cloudmonkey/cmd.init.4.func1(0xc0011ab740)
/home/shwstppr/lab/shapeblue/cloudstack-cloudmonkey/cmd/sync.go:35 +0xd0
github.com/apache/cloudstack-cloudmonkey/cli.ExecCmd({0xc001330d30, 0x1, 0x1}, 0x0)
/home/shwstppr/lab/shapeblue/cloudstack-cloudmonkey/cli/exec.go:64 +0x1bb
github.com/apache/cloudstack-cloudmonkey/cli.ExecLine({0xc00133d420, 0x4})
/home/shwstppr/lab/shapeblue/cloudstack-cloudmonkey/cli/exec.go:51 +0x21b
github.com/apache/cloudstack-cloudmonkey/cli.ExecPrompt()
/home/shwstppr/lab/shapeblue/cloudstack-cloudmonkey/cli/prompt.go:84 +0x26e
main.main()
/home/shwstppr/lab/shapeblue/cloudstack-cloudmonkey/cmk.go:101 +0x7f1
@kiranchavala no user with that api key, re-check with valid key. cmk also need a fix to address this error: (created issue: apache/cloudstack-cloudmonkey#211) |
492b5d1 to
aa6f16b
Compare
|
@blueorangutan package |
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17930 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
winterhazel
added
status:waiting-for-author
and removed
status:ready-for-merge
status:waiting-for-smoketest-results
labels
@sureshanaparti ping. It would be nice to include this fix in 4.23 |
|
@blueorangutan package |
There was a problem hiding this comment.
Pull request overview
This PR addresses a NullPointerException when listApis is called with an API key that does not resolve to an ApiKeyPair, by adding null-handling in API key pair lookup paths within AccountManagerImpl.
Changes:
- Add null checks when resolving an accessing
ApiKeyPairto prevent NPEs. - Handle missing
ApiKeyPairresults in permission-resolution logic and improve related logging. - Fix
getUserByApiKeyto avoid dereferencing a null key pair.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@blueorangutan package |
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
| ApiKeyPair accessingKeyPair = apiKeyPairService.findByApiKey(accessingApiKey); | ||
| if (userId == accessingKeyPair.getUserId()) { | ||
| if (accessingKeyPair != null && userId == accessingKeyPair.getUserId()) { | ||
| keyPair = apiKeyPairService.findByApiKey(accessingApiKey); |
There was a problem hiding this comment.
As for this suggestion, we could apply it in this PR, if you're willing to, @sureshanaparti. But it is nitpicking IMO
| } catch (Exception e) { | ||
| logger.info("Error accessing API through session.", e); | ||
| } |
There was a problem hiding this comment.
This Copilot suggestion does not make sense in the context of this method
| if (apiKeyPair == null) { | ||
| logger.warn("Unable to find API key pair by API key."); | ||
| return new ArrayList<>(); | ||
| } |
| public List<RolePermissionEntity> getAllKeypairPermissions(String apiKey) { | ||
| if (apiKey == null) { | ||
| throw new InvalidParameterValueException("API key not present in the request's URL and, thus, unable to fetch API key rules."); | ||
| } | ||
| ApiKeyPair apiKeyPair = keyPairManager.findByApiKey(apiKey); |
There was a problem hiding this comment.
This does not need to be applied in this PR
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 18495 |
| if (accessingApiKey != null) { | ||
| ApiKeyPair accessingKeyPair = apiKeyPairService.findByApiKey(accessingApiKey); | ||
| if (userId == accessingKeyPair.getUserId()) { | ||
| if (accessingKeyPair != null && userId == accessingKeyPair.getUserId()) { |
There was a problem hiding this comment.
It might be better to throw an exception here, if apikeypair is not found
similar to change with getAllKeypairPermissions method
There was a problem hiding this comment.
| if (accessingKeyPair != null && userId == accessingKeyPair.getUserId()) { | |
| if (accessingKeyPair == null) { | |
| throw new CloudRuntimeException(String.format("Unable to find pair of API key [%s].", accessingApiKey)); | |
| } | |
| if (userId == accessingKeyPair.getUserId()) { |
@sureshanaparti it makes more sense to throw an exception here. Although unlikely, the way this if is structured right now can result in the key of user A of account X leaking when the caller is using a key to access user B of account X if accessingKeyPair is null.
| } | ||
| ApiKeyPair apiKeyPair = keyPairManager.findByApiKey(apiKey); | ||
| if (apiKeyPair == null) { | ||
| logger.warn("Unable to find API key pair by API key."); |
There was a problem hiding this comment.
@sureshanaparti I would throw a CloudRuntimeException here as well. Even though this does not represent a security concern, returning an empty list here will allow the API call to finish "successfully", hiding an issue.
|
@sureshanaparti |
|
@winterhazel, before merging this, could you review it again, please? |
|
@winterhazel @bernardodemarco @sureshanaparti |
@weizhouapache I don't think so, although it would be nice to fix it for 4.23. I was only able to reproduce it by configuring valid username + password credentials for a profile in CMK, alongside an invalid API key. (test) 🐱 > set apikey WpsOEhOzpJHDlo-RM7yezDdj5I_0o10GsqwCa9SF8sW8VeGHXLUY0X2THwOHNR7mVWup02SpS6-7JdCJrGYtNw
Loaded in-built API cache. Failed to read API cache, please run 'sync'.
(test) 🐱 > set secretkey GRiS1DJ7ToI1r8bJ5VDdzCAx_KSrnfcr3GZeMLEm0bNdTSSOe852_wHW74YdER_7mbShn09D-E5NoYxejQ6t_Q
Loaded in-built API cache. Failed to read API cache, please run 'sync'.
(test) 🐱 > sync
Discovered 889 APIs
(test) 🐱 > set apikey WpsOEhOzpJHDlo-RM7yezDdj5I_0o10GsqwCa9SF8sW8VeGHXLUY0X2THwOHNR7mVWup02SpS6-7JdCJrGYtN
(test) 🐱 > sync
🙈 Error: failed to authenticate, please check the credentials
(test) 🐱 > set username admin
(test) 🐱 > sync
🙈 Error: failed to authenticate, please check the credentials
(test) 🐱 > set password password
(test) 🐱 > sync
🙈 Error: (HTTP 530, error code 9999) Cannot invoke "org.apache.cloudstack.acl.apikeypair.ApiKeyPair.getAccountId()" because "apiKeyPair" is nullUsing either (1) only username + password, (2) only API key + secret key, (3) username + password + valid API key does not result in any exceptions. |
cool, thanks Since this is not a critical issue, 4.23 RC will not be blocked by it |
|
@bernardodemarco , you are marked as requesting changes, can you check? |
|
@weizhouapache should we merge for rc2? (assuming we get one) |
Hello, @DaanHoogland Yes, @winterhazel and I have already reviewed the most recent changes of the PR. We are awaiting for updates to review again |
@DaanHoogland assuming that it is ready for merge before the creation of RC2, yes. It is not a blocker issue though. @sureshanaparti are you able to work on the requested changes this week? |
Description
This PR fixes NPE issue with ApiKeyPair during listApis call (from cmk).
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?